[PIX] Add validation support to PIX pass tests - #8841
Open
Damyan Pepper (damyanp) wants to merge 8 commits into
Open
[PIX] Add validation support to PIX pass tests#8841Damyan Pepper (damyanp) wants to merge 8 commits into
Damyan Pepper (damyanp) wants to merge 8 commits into
Conversation
The PIX passes change DXIL after the compiler completes. A pass can add a resource, change a root signature, insert an operation, and remove a declaration. The result can be a module that the validator refuses. The tests only examine the disassembly, so they cannot find this type of defect. The new helpers run the DXIL validator on the output of a pass. They also separate the diagnostics that PIX instrumentation is permitted to cause from the diagnostics that are defects. The set of permitted diagnostics decides what every later layer can ignore, so it is the part to examine with care. There is no change to the compiler. Assisted-by: Copilot Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40dc9de3-617e-4caf-ab0d-fba0a033ed93
Damyan Pepper (damyanp)
requested a review
from Austin Kinross (austinkinross)
August 27, 2026 21:05
Damyan Pepper (damyanp)
force-pushed
the
users/damyanp/pix-fixes-01
branch
from
August 27, 2026 21:08
a947233 to
5238ee0
Compare
Damyan Pepper (damyanp)
marked this pull request as ready for review
August 27, 2026 22:43
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a validation harness for PIX pass test outputs.
Changes:
- Adds single-pass execution and DXIL validation helpers.
- Filters permitted PIX metadata diagnostics.
- Adds control tests for valid and invalid outputs.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Austin Kinross (austinkinross)
approved these changes
Aug 27, 2026
Joshua Batista (bob80905)
approved these changes
Aug 29, 2026
Virtual-register annotation attaches metadata that DXIL does not consume, so validation of an instrumented module always reported a generic "unused metadata" diagnostic even when the module was otherwise correct. The prior handling matched that diagnostic by substring, which would also swallow a genuinely unrelated unused-metadata defect. Replace it with a structural check: on direct validation failure, clone the module, strip only the four known PIX virtual-register metadata kinds, and revalidate. Accept only if the stripped clone validates, proving PIX metadata was the sole cause. Rework Validation_ControlInvalidModuleFails so the corrupted container itself proves both facts independently: direct validation's diagnostic confirms the permitted PIX metadata is present and unused, and the harness's rejection confirms a real, non-boilerplate defect remains. Replace Validation_ControlBoilerplateOnlyFailureIsRejected, which exercised the removed string classifier, with Validation_ControlNonPixUnusedMetadataIsRejected: a foreign metadata kind alongside the module's own PIX metadata must still be rejected after the four-kind strip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Apply the repository's almost-never-auto convention to every remaining auto introduced by the L1 original and direct-feedback commits. Use the declared Compile and RunSinglePass result types and std::string::size_type for the find result. No behavior changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Fresh container reconstruction can hide container-level validation defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Container reconstruction can mask genuine root-signature or PSV validation failures.
Review details
Suppressed comments (1)
tools/clang/unittests/HLSL/PixTest.cpp:400
- Reassembling the stripped module changes more than the permitted PIX metadata: the assembler regenerates derived parts such as PSV/signatures and this clone path does not restore container-only state such as RTS0/RDAT. Therefore, if direct validation reports both unused PIX metadata and a real container/module mismatch (for example an incompatible root signature or stale PSV), the second validation can see the mismatch repaired or removed and incorrectly return
Valid. Preserve the original non-module container parts while replacing only the metadata-stripped DXIL payload, and add a control case for a container-part mismatch.
CComPtr<IDxcBlob> strippedContainer =
CloneModuleAndMutate(pContainer, StripKnownPixVirtualRegisterMetadata);
if (RunValidator(strippedContainer).Valid) {
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Use Capitalized names for the parameters and local variables added by the PIX validation harness. Remove Hungarian prefixes without changing older code in PixTest.cpp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8
Rename the PIX validation helpers to verb phrases that start with a lowercase letter, as required by the LLVM coding standards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The metadata allowlist can conceal malformed or misplaced PIX metadata.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
Reject known PIX metadata with an invalid attachment location or payload before removing it for DXIL validation. Add controls for function metadata, metadata on the wrong instruction type, and malformed metadata. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 819bcd16-83fc-4619-ac1a-b12ee50a2fe8
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The updated harness narrowly permits validated PIX metadata, rejects the covered invalid cases, and appropriately requires no release note for test-only changes.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PIX passes change DXIL after the compiler completes. A pass can add a resource, change a root signature, insert an operation, and remove a declaration. The result can be a module that the validator refuses. The tests only examine the disassembly, so they cannot find this type of defect.
The new helpers run the DXIL validator on the output of a pass. They also separate the diagnostics that PIX instrumentation is permitted to cause from the diagnostics that are defects. The set of permitted diagnostics decides what every later layer can ignore, so it is the part to examine with care.
There is no change to the compiler.
Assisted-by: Copilot
Stack created with GitHub Stacks CLI • Give Feedback 💬